Skip to content

Conversation

@0xStuart
Copy link

Required for complaint WSS support. I haven't the hardware to test this right now.

@jeremypoulter jeremypoulter requested a review from Copilot October 14, 2025 15:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds TLS certificate configuration and SNI (Server Name Indication) support for OCPP WebSocket connections to enable compliant WSS (WebSocket Secure) support. The changes introduce certificate management functionality with configurable security policies for OCPP connections.

Key Changes

  • Added TLS configuration structure and methods to manage certificates and security policies
  • Introduced certificate ID configuration parameter for selecting specific certificates
  • Added flag-based configuration for rejecting unauthorized certificates

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/ocpp.h Added TLS configuration structure, certificate management methods, and state tracking fields
src/ocpp.cpp Implemented TLS configuration logic, certificate resolution, and security policy application
src/app_config.h Added certificate ID configuration variable and security flag definitions
src/app_config.cpp Added configuration options for certificate ID and certificate validation settings

src/ocpp.cpp Outdated
}

TlsConfig desired = resolveTlsConfig();
bool changed = !tlsConfigApplied || desired.rejectUnauthorized != appliedRejectUnauthorized || desired.caCert != appliedCaCert;
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointer comparison for desired.caCert != appliedCaCert compares memory addresses rather than string content. This will incorrectly detect changes when the same certificate content is stored at different memory locations. Use strcmp() or similar string comparison instead.

Copilot uses AI. Check for mistakes.
src/ocpp.cpp Outdated
#include <MicroOcpp/Model/Diagnostics/DiagnosticsService.h>
#include <MicroOcpp/Model/FirmwareManagement/FirmwareService.h>
#include <MongooseCore.h>

Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Include <cstdlib> should be placed with other system includes at the top, before the blank line that separates system includes from local includes.

Suggested change

Copilot uses AI. Check for mistakes.
@jeremypoulter
Copy link
Collaborator

Thank you for your contribution, I will take a look

@jeremypoulter
Copy link
Collaborator

@0xStuart Have you made appropriate UI updates to alloe the certificate to be set? Also please check the CoPilot comments

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@0xStuart
Copy link
Author

@jeremypoulter
As far as I know the general root CA bundle is built by buildRootCa() and used when no specific cert is chosen.
I'm not very familiar with the codebase. I wouldn't have thought any additional UI was necessary.
As I say, I don't have any hardware to test this right now.

@jeremypoulter
Copy link
Collaborator

buildRootCa() will combine all the root certificats, those compiled and those uploaded by the user, so if your goal is to just allow additional certificates then you do not need any of the changes to the config, or seelction of a specific OCPP certificate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants